Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactoring eniconfig func to only take node as parameter #2387

Merged
merged 1 commit into from
May 17, 2023
Merged

refactoring eniconfig func to only take node as parameter #2387

merged 1 commit into from
May 17, 2023

Conversation

haouc
Copy link
Contributor

@haouc haouc commented May 17, 2023

What type of PR is this?
Refactor a function in eniconfig package

exposing an API to VPC resource controller
Which issue does this PR fix:

What does this PR do / Why do we need it:
This PR refactor the function GetNodeSpecificENIConfigName which can exposes an API for VCP resource controller to call as a dependency.

If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:

Testing done on this change:

custom networking labels were tested after refactoring.

% k get no -oyaml | grep eniConfig
      vpc.amazonaws.com/eniConfig: us-west-2b
      vpc.amazonaws.com/eniConfig: us-west-2b
      vpc.amazonaws.com/eniConfig: us-west-2a
      vpc.amazonaws.com/eniConfig: us-west-2b

Automation added to e2e:

no change on functionality
Will this PR introduce any new dependencies?:

no
Will this break upgrades or downgrades. Has updating a running cluster been tested?:
no

Does this change require updates to the CNI daemonset config files to work?:

no
Does this PR introduce any user-facing change?:

no

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@haouc haouc requested a review from a team as a code owner May 17, 2023 20:16
@haouc haouc requested review from jayanthvn and jdn5126 May 17, 2023 20:17
jdn5126
jdn5126 previously approved these changes May 17, 2023
View reviewed changes
@haouc haouc force-pushed the master branch 3 times, most recently from 8f83564 to 758e54a Compare May 17, 2023 21:38
@haouc haouc merged commit 2eef830 into aws:master May 17, 2023
@FernandoMiguel
Copy link

not sure if related, but this is the only change regarding ENIConfig i could find in vpccni v1.13
on v1.12 this works fine.
on 1.13 we are seeing issues when applying terraform

Error: us-east-1a failed to fetch resource from kubernetes: the server could not find the requested resource
  with module.base_system.kubectl_manifest.eni_config["subnet-XXXX"],
  on .terraform/modules/base_system/cni_custom_networking.tf line 5, in resource "kubectl_manifest" "eni_config":
   5: resource "kubectl_manifest" "eni_config" {

is there a chance this change introduced a breaking change @haouc @jdn5126 ?

@jdn5126
Copy link
Contributor

jdn5126 commented Jun 27, 2023

@FernandoMiguel are you using ENIConfigs without any security group specified? This was one known bug between v1.12.x and 1.13.0: #2426

If this is not the case, or if you are using v1.13.2, let's open a new issue or connect on #aws-vpc-cni on k8s Slack

@FernandoMiguel
Copy link

@jdn5126 there is one sg per ENI config.
I'll jump on slack tomorrow

@FernandoMiguel
Copy link

after further investigating this, seems we are being affected by this instead
gavinbunney/terraform-provider-kubectl#270
sorry to have bothered you

@jdn5126
Copy link
Contributor

jdn5126 commented Jun 28, 2023

@FernandoMiguel no worries, glad you have a root cause!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdn5126 jdn5126 jdn5126 approved these changes

@jayanthvn jayanthvn Awaiting requested review from jayanthvn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@haouc @FernandoMiguel @jdn5126